Switch from Rf_findVarInFrame to R_getVarEx#1423
Conversation
kevinushey
left a comment
kevinushey
left a comment
There was a problem hiding this comment.
Some nits around the value we pass for inherits.
| #if R_VERSION < R_Version(4,5,0) | ||
| Rcpp_cache = Rf_findVarInFrame(RCPP, Rf_install(".rcpp_cache")); | ||
| #else | ||
| Rcpp_cache = R_getVar(Rf_install(".rcpp_cache"), RCPP, TRUE); |
There was a problem hiding this comment.
| Rcpp_cache = R_getVar(Rf_install(".rcpp_cache"), RCPP, TRUE); | |
| Rcpp_cache = R_getVar(Rf_install(".rcpp_cache"), RCPP, FALSE); |
There was a problem hiding this comment.
This one I was more torn about. Note the comment just above the code:
/**
* Get an object from the environment or one of its
* parents
*
* @param name name of the object
*
*/
SEXP find( const std::string& name) const{
So does is walk through the other envs?
Also note that lines 139 to 162 (for find(std::string)) and 164 to 188 (for find(Symbol)) should be symmetric. Both TRUE, or both FALSE?
There was a problem hiding this comment.
I'm not sure if I'm missing something, but isn't this code just trying to get a reference to the .rcpp_cache variable in the Rcpp namespace?
There was a problem hiding this comment.
I think the prior edits are preserving that behavior you suggest, though -- get => inherits = FALSE; find => inherits = TRUE.
There was a problem hiding this comment.
I think that is where we are at right now (i.e.: get has a FALSE, find has a TRUE) and that is due to one correction made after your comment made days ago (as noted above). And that seems fine.
(The 'just look for .rcpp_cache' is in a different file and seemingly a debugging-only helper.)
eddelbuettel
changed the title
eddelbuettel
force-pushed
the
feature/use_R_getVar
branch
from
7636fec to
3f4ff02
Compare
|
Rebased once more |
|
@kevinushey Do you want to take another look an |
|
Reverse-depends checks turned up no new issues so all good from there too |
2 participants
Closes #1421
As described in #1421, this is another upcoming R API tightening. This fairly narrow PR does this, keeping the old code til we can remove it a few years from now and switches to the new functions for R 4.5.0 or newer.
We should leave this open until the reverse dependency checks give us a view into what the impact may be.
Checklist
R CMD checkstill passes all tests